Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JackPosixProcessSync: Fix mutex owner on TimedWait() timeout. #952

Merged
merged 1 commit into from
Dec 9, 2023

Conversation

0EVSG
Copy link
Contributor

@0EVSG 0EVSG commented Jul 30, 2023

Per POSIX definition, pthread_cond_timedwait() re-acquires the mutex when a timeout occurs and ETIMEDOUT is returned. In that case also mark the waiting thread as owner again, for consistency. Otherwise subsequent attempts to unlock the mutex will fail, leaving the mutex locked forever.

The return value of TimedWait() is still false on timeout, the same as for other platform's implementations.
But AFAICT, JackPosixProcessSync::TimedWait() is only used by my new FreeBSD OSS backend, see #943.

Per POSIX definition, pthread_cond_timedwait() re-acquires the mutex
when a timeout occurs and ETIMEDOUT is returned. In that case also
mark the waiting thread as owner again, for consistency.
Otherwise subsequent attempts to unlock the mutex will fail, leaving
the mutex locked forever.
@0EVSG 0EVSG added the bug label Jul 30, 2023
@0EVSG 0EVSG requested review from falkTX and sletz July 30, 2023 23:34
@0EVSG 0EVSG self-assigned this Jul 30, 2023
@0EVSG
Copy link
Contributor Author

0EVSG commented Dec 9, 2023

Same here, should I just merge this if nobody's got time for a review?

@falkTX
Copy link
Member

falkTX commented Dec 9, 2023

Would be nice to confirm with some automated testing that we do not get regressions, but since as you say this is only used for BSD the breakge is more minimal and I can assume you tested it.
Feel free to merge yes

@0EVSG 0EVSG merged commit 886b35c into jackaudio:develop Dec 9, 2023
@0EVSG
Copy link
Contributor Author

0EVSG commented Dec 9, 2023

It's difficult to test for regression when the code isn't even in use ;-)

I only discovered this problem when testing #943 - the timed wait treated a timeout like an error. Which left the mutex locked, but with ownership unset, and thus prevented any further unlocking of the mutex. I'm pretty sure that behavior was wrong and this patch at least improved it. Thread synchronization has been robust since then, so I'm gonna merge.

@0EVSG 0EVSG deleted the posix_timed_wait branch December 9, 2023 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants